fix: detect ScyllaDB via SUPPORTED protocol extensions, not shard count#910
fix: detect ScyllaDB via SUPPORTED protocol extensions, not shard count#910mykaul wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
participant ProtocolFeatures
participant ControlConnection
participant SchemaParserV3
ProtocolFeatures->>ProtocolFeatures: parse_from_supported(supported)
ProtocolFeatures->>ProtocolFeatures: detect_scylla(supported, sharding_info)
ProtocolFeatures-->>ControlConnection: is_scylla
ControlConnection->>ControlConnection: set _uses_peers_v2 and metadata timeout
ControlConnection->>SchemaParserV3: _is_not_scylla(features)
SchemaParserV3-->>ControlConnection: not is_scylla
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mykaul
force-pushed
the
fix-scylla-detection-without-sharding
branch
from
a64d05e to
bea45ac
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes ScyllaDB detection by using Scylla-specific SUPPORTED protocol extension keys (and sharding info when present) instead of relying on shard-related fields alone, preventing misclassification as Cassandra when shard-awareness is disabled and avoiding peers_v2-related connection failures.
Changes:
- Add
ProtocolFeatures.is_scyllaand implement Scylla detection via known Scylla-specificSUPPORTEDextension keys. - Harden sharding parsing/initialization against missing shard fields to prevent
int(None)crashes. - Gate peers_v2 usage,
USING TIMEOUT, and trigger-metadata behavior onis_scyllainstead of shard-awareness heuristics; add unit tests for detection/regressions.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
cassandra/protocol_features.py |
Introduces is_scylla detection and adjusts sharding parsing to avoid crashes. |
cassandra/shard_info.py |
Guards integer conversion for missing shard-related fields. |
cassandra/c_shard_info.pyx |
Mirrors shard-info conversion guards in the Cython implementation. |
cassandra/cluster.py |
Switches peers_v2 disabling and metadata timeout gating to is_scylla. |
cassandra/metadata.py |
Updates Scylla-vs-not logic to use features.is_scylla. |
tests/unit/test_protocol_features.py |
Adds unit tests for Scylla detection and crash regressions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| resolved_shard_id = int(shard_id) if shard_id is not None else 0 | ||
| return resolved_shard_id, _ShardingInfo(shard_id, shards_count, partitioner, sharding_algorithm, sharding_ignore_msb, | ||
| shard_aware_port, shard_aware_port_ssl) |
| # SCYLLA_PARTITIONER passes the sharding guard, so sharding_info is populated | ||
| # with zero defaults rather than crashing. | ||
| assert pf.sharding_info is not None | ||
| assert pf.sharding_info.shards_count == 0 | ||
| assert pf.sharding_info.sharding_ignore_msb == 0 |
| assert pf.is_scylla is True | ||
| assert pf.sharding_info is not None | ||
| assert pf.sharding_info.shards_count == 0 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_protocol_features.py (1)
79-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the normalized
pf.shard_idin these regressions.These cases exercise the missing-
SCYLLA_SHARDpath, but they never check the new top-level normalization to0. Addingassert pf.shard_id == 0would pin the actual fix and catch a regression back toNone.Suggested assertions
def test_scylla_without_sharding_no_crash(self): ... assert pf.is_scylla is True + assert pf.shard_id == 0 assert pf.sharding_info is not None assert pf.sharding_info.shards_count == 0 assert pf.sharding_info.sharding_ignore_msb == 0 def test_scylla_sharding_algorithm_only_no_crash(self): ... assert pf.is_scylla is True + assert pf.shard_id == 0 assert pf.sharding_info is not None assert pf.sharding_info.shards_count == 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_protocol_features.py` around lines 79 - 108, The regression tests in test_scylla_without_sharding_no_crash and test_scylla_sharding_algorithm_only_no_crash only verify sharding_info, but they do not assert the new normalized top-level shard_id value. Update both ProtocolFeatures.parse_from_supported regression cases to also check that pf.shard_id is 0, so the tests pin the missing-SCYLLA_SHARD normalization behavior and catch any return to None.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/test_protocol_features.py`:
- Around line 79-108: The regression tests in
test_scylla_without_sharding_no_crash and
test_scylla_sharding_algorithm_only_no_crash only verify sharding_info, but they
do not assert the new normalized top-level shard_id value. Update both
ProtocolFeatures.parse_from_supported regression cases to also check that
pf.shard_id is 0, so the tests pin the missing-SCYLLA_SHARD normalization
behavior and catch any return to None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e195da2-f03f-4a94-a124-f7657f65a989
📒 Files selected for processing (6)
cassandra/c_shard_info.pyxcassandra/cluster.pycassandra/metadata.pycassandra/protocol_features.pycassandra/shard_info.pytests/unit/test_protocol_features.py
The driver previously identified ScyllaDB solely by the presence of shard-related fields (SCYLLA_NR_SHARDS, SCYLLA_SHARD, etc.) in the SUPPORTED response. When shard-awareness is disabled on the server side (allow_shard_aware_drivers: false), those fields are absent, causing the driver to misidentify a ScyllaDB cluster as Cassandra. Changes: - Add ProtocolFeatures.detect_scylla() and is_scylla flag, set from known Scylla-specific extension keys (LWT, rate-limit, tablets routing) or sharding_info presence - Replace sharding_info check with is_scylla for peers_v2 and metadata_request_timeout in cluster.py and metadata.py - Guard int() calls against None in _ShardingInfo construction (c_shard_info.pyx, shard_info.py) - Add tests for is_scylla detection and regression tests for missing shard fields - Assert pf.shard_id normalization in test regressions (response to CodeRabbit review)
mykaul
force-pushed
the
fix-scylla-detection-without-sharding
branch
from
bea45ac to
b223706
Compare
| """Check if NOT connected to ScyllaDB. | ||
|
|
||
| Uses the is_scylla flag from ProtocolFeatures, which is set from the | ||
| presence of Scylla-specific extension keys in the SUPPORTED response | ||
| (e.g. SCYLLA_LWT_ADD_METADATA_MARK, SCYLLA_RATE_LIMIT_ERROR) and | ||
| therefore remains True even when shard-awareness is disabled. | ||
| """ | ||
| return not getattr(getattr(self.connection, 'features', None), 'is_scylla', False) |
There was a problem hiding this comment.
ProtocolFeatures always have is_scylla attribute after your changes, why do you need getattr here?
Also, is it possible for connection to not have features?
There was a problem hiding this comment.
It was mostly a copy-paste from the original code, IIRC. I'll look into it.
(I'm also unsure - what if self.connection is null?)
| # SCYLLA_SHARD may be absent even when other shard fields are present | ||
| # (e.g. the connection landed on shard 0 and the server omits the field). | ||
| # Default to 0 to avoid int(None) crash. | ||
| resolved_shard_id = int(shard_id) if shard_id is not None else 0 | ||
| return resolved_shard_id, _ShardingInfo(shard_id, shards_count, partitioner, sharding_algorithm, sharding_ignore_msb, | ||
| shard_aware_port, shard_aware_port_ssl) |
There was a problem hiding this comment.
Can SCYLLA_SHARD be absent, as this comment indicates?
protocol-extensions.md doesn't mention it being optional. Scylla code also seems to insert it unconditionally: https://github.com/scylladb/scylladb/blob/b9d508d4097a91181da582305900c9318a9fec50/transport/server.cc#L2046-L2058
There was a problem hiding this comment.
Yes, that's the whole issue that started this thread - if Scylla server-side disables sharding, it doesn't advertise it whatsoever.
There was a problem hiding this comment.
That is not what I'm asking.
SCYLLA_SHARD may be absent even when other shard fields are present
Afaik disabling sharding on scylla-side will make it not send ANY sharding-related values in SUPPORTED, which makes this comment false. See the code fragment I linked - all sharding related values are guarded by _server._config.allow_shard_aware_drivers. I don't see a possible scenario where SCYLLA_SHARD is absent but e.g. SCYLLA_NR_SHARDS is present.
|
|
||
| def __init__(self, shard_id, shards_count, partitioner, sharding_algorithm, sharding_ignore_msb, shard_aware_port, shard_aware_port_ssl): | ||
| self.shards_count = int(shards_count) | ||
| self.shards_count = int(shards_count) if shards_count else 0 | ||
| self.partitioner = partitioner | ||
| self.sharding_algorithm = sharding_algorithm | ||
| self.sharding_ignore_msb = int(sharding_ignore_msb) | ||
| self.sharding_ignore_msb = int(sharding_ignore_msb) if sharding_ignore_msb else 0 | ||
| self.shard_aware_port = int(shard_aware_port) if shard_aware_port else None | ||
| self.shard_aware_port_ssl = int(shard_aware_port_ssl) if shard_aware_port_ssl else None |
There was a problem hiding this comment.
I don't think it is possible for shards_count / sharding_ignore_msb when other values are present. Either sharding info is enabled, in which those values are present, or it is not, in which case we should not attempt to construct _ShardingInfo object. In both cases, those new guards are unnecessary.
There was a problem hiding this comment.
So do you want me to change the code to check for everything works with or without _ShardingInfo? I thought it might be a slightly bigger change. I can look into it.
Summary
The driver previously identified ScyllaDB solely by the presence of shard-related fields (
SCYLLA_NR_SHARDS,SCYLLA_SHARD, etc.) in theSUPPORTEDresponse.When shard-awareness is disabled on the server side (
allow_shard_aware_drivers: false), those fields are absent, causing the driver to misidentify a ScyllaDB cluster as Cassandra.The immediate consequence is that the driver leaves
peers_v2enabled (which ScyllaDB does not support) and fails to connect — the same regression described in scylladb/gocql#902.This fix mirrors scylladb/gocql#903.
Changes
cassandra/protocol_features.pyProtocolFeatures.is_scyllabool (defaultFalse), set by the newdetect_scylla()static method.detect_scylla()returnsTruewhen any known Scylla-specific extension key (SCYLLA_LWT_ADD_METADATA_MARK,SCYLLA_RATE_LIMIT_ERROR,TABLETS_ROUTING_V1) is present in theSUPPORTEDresponse, or whensharding_infois populated.These keys are advertised by ScyllaDB regardless of whether shard-awareness is enabled, so
is_scyllastaysTrueeven with sharding disabled.parse_sharding_info:int(shard_id)raisedTypeErrorwhenSCYLLA_PARTITIONER/SCYLLA_SHARDING_ALGORITHMwas present withoutSCYLLA_SHARD. Default to0.cassandra/shard_info.py/cassandra/c_shard_info.pyx_ShardingInfo.__init__/ShardingInfo.__init__: guardint(shards_count)andint(sharding_ignore_msb)againstNone(default to0), matching the existing pattern used forshard_aware_port. Fixes the remaining crash path where a shard-defining field was present withoutSCYLLA_NR_SHARDS.cassandra/cluster.py_uses_peers_v2disable and_metadata_request_timeoutnow gate onis_scyllainstead ofsharding_info is not None, so both work correctly when sharding is disabled.cassandra/metadata.py_is_not_scylla()now usesnot is_scyllainstead ofshard_id is None.The old check was always
Falseafter the OPTIONS exchange (Cassandra getsshard_id=0, and0 is NoneisFalse), meaning trigger-metadata queries were silently skipped even for Cassandra/DSE connections.tests/unit/test_protocol_features.pyint(None)paths.Shard-aware pooling is unaffected
pool.pycontinues to gate per-shard connection opening onsharding_info is not None.A ScyllaDB node with sharding disabled correctly gets
is_scylla=True(so peers_v2 is disabled and USING TIMEOUT works) butsharding_info=None(so no per-shard connections are opened). This matches the gocql#903 fix'sisScyllaConn() && nrShards != 0guard.Note on Cassandra behavioral change
_is_not_scylla()was previously always returningFalsepost-OPTIONS for Cassandra (shard_id=0,0 is None = False), sosystem_schema.triggersqueries were silently skipped even for Cassandra/DSE. With this fix, Cassandra/DSE connectionswill correctly issue the trigger-metadata query and populate
TableMetadata.triggers.See also